• fix(pegboard): cache none runner pool errors too #3899
• fix(engine-runner): handle shutdown close codes correctly #3896
• chore(gas): add overview and history docs #3879
• fix(otel): enrich http traces #3878
• fix(guard): clean up guard #3862
• fix: add ready_chunks to worker bumps, bumps per tick metric #3855 👈 (View in Graphite)
• fix(epoxy): add metrics #3847
• chore: clean up errors #3846
• fix(pb): have restart policy actors sleep after pending for too long #3837
• fix: set failure reason regardless of crash policy #3835
• feat: add metrics to pb, guard #3815
• fix: clear all pending signals after workflow complete #3798
• fix: remove pending actors metric, fix actor error tracker, engine runner error print #3797
• chore(engine): add support for epoxy debugging #3794
• main

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

This stack of pull requests is managed by Graphite. Learn more about stacking.

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3855

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3855

commit: c46083b

PR Review: Add ready_chunks to worker bumps, bumps per tick metric

Summary

This PR introduces two improvements to the Gasoline worker's bump handling:

1. Uses ready_chunks(1024) to batch bump messages together
2. Adds a new metric WORKER_BUMPS_PER_TICK to track how many bump messages are processed per tick

Positive Observations

✅ Good performance optimization: Using ready_chunks is an excellent approach to coalesce multiple bump notifications into a single wake event. This prevents unnecessary tick processing when many bumps arrive in quick succession.

✅ Useful observability: The new histogram metric will provide valuable insights into bump batching behavior and help identify if the chunk size of 1024 is appropriate.

✅ Proper histogram buckets: The bucket distribution (1, 2, 3, 4, 8, 16, 32, 64, 128, 256, 512, 1024) is well-suited for tracking bump counts with good granularity at lower values.

✅ Follows project conventions: Code style matches the existing patterns, structured logging is used properly, and the metric is registered correctly.

Issues and Concerns

1. Logic Error: Metric only recorded on Some, not None (Minor)

In worker.rs:112-118, the metric is only observed when bumps are received (Some(bumps)). However, the tick still happens and processes workflows even when the subscription ends (None case), which immediately breaks the loop. This is likely fine, but worth noting that the metric won't capture the final state before shutdown.

2. Missing Logging (Minor)

The original code didn't log anything about bumps, and neither does the new code. Consider adding a debug log when a large batch is received to help with diagnostics.

3. Magic Number: 1024 chunk size (Minor)

The chunk size of 1024 is hardcoded without explanation. While this is likely fine for most cases, consider:

• Documenting why 1024 was chosen
• Whether this should be configurable
• What happens if the actual burst size exceeds 1024 (answer: it processes them in multiple ticks, which is fine)

4. Ready Chunks Behavior Clarification (Documentation)

The comment on line 77 states "multiple bumps in a row should be processed as 1 bump," which is accurate. However, it's worth understanding that ready_chunks(1024):

• Returns immediately with available items (up to 1024)
• Does NOT wait to fill the buffer
• This is the desired behavior for responsiveness

The comment is correct, but could be clearer about the immediate-return behavior.

Recommendations

Priority: Low

1. Consider adding a comment explaining the choice of 1024 as the chunk size
2. Optional: Add debug logging for large bump batches to help with diagnostics
3. Optional: Add a test case that verifies metric recording with bump batching

Code Quality Assessment

• Style: ✅ Follows project conventions
• Error Handling: ✅ Appropriate (continues on None by breaking)
• Performance: ✅ Improvement from baseline
• Observability: ✅ Good metric added
• Security: ✅ No concerns
• Test Coverage: ⚠️ No new tests, but this is observability code

Verdict

LGTM with minor suggestions - This is a solid improvement that adds both performance optimization and observability. The issues noted are minor and mostly documentation-related. The code is ready to merge as-is, though the suggestions above would make it even better.

Review generated by Claude Code

PR Review: Add ready_chunks to worker bumps, bumps per tick metric

Summary

This PR adds observability for worker bump message batching by using ready_chunks(1024) on the bump subscription stream and tracking the number of bumps received per tick via a new Prometheus histogram metric.

✅ Strengths

1. Good observability addition: The new WORKER_BUMPS_PER_TICK metric will help understand bump message batching patterns and worker load
2. Appropriate bucket choices: The histogram buckets (1, 2, 3, 4, 8, 16, 32, 64, 128, 256, 512, 1024) cover the full range of the ready_chunks limit and provide good granularity for analysis
3. Correct metric usage: The metric is properly recorded with the worker ID label for per-worker tracking

🔍 Issues & Concerns

1. Logic Error: Metrics recorded only for Some(bumps), not all ticks (worker.rs:110-120)

The current implementation only observes the metric when bumps are received (Some(bumps)), but not when the subscription returns None. This could skew the metric distribution since we won't record zero-bump events from other tick sources.

However, looking more closely at the code flow: when bump_sub.next() returns None, the worker breaks with an error. So this case represents a fatal error condition, not a normal tick with zero bumps.

But there's still an issue: The tick can also be triggered by tick_interval.tick() (line 109), and in that case, no metric is recorded at all. This means the metric only captures ticks triggered by bump messages, not periodic polling ticks. This could be misleading for understanding overall worker tick patterns.

Recommendation: Consider whether you want to track bumps per tick for ALL ticks (including timer-based ticks, which would have 0 bumps), or only for bump-triggered ticks. If the former, you'll need to move the metric observation outside the tokio::select! or add it to both branches.

2. ready_chunks buffer size lacks justification (worker.rs:82)

The hardcoded value of 1024 for ready_chunks appears arbitrary. Consider:

• What is the expected burst rate of bump messages?
• Is 1024 appropriate for all deployment scenarios?
• Should this be configurable?

Recommendation: Add a comment explaining why 1024 was chosen, or better yet, make it a configuration parameter.

3. Potential memory concern with ready_chunks

The ready_chunks(1024) will buffer up to 1024 messages in memory. Each message is a () unit type (based on the type signature BoxStream<'b, ()>), so memory impact is minimal. However, the comment states "multiple bumps in a row should be processed as 1 bump" (worker.rs:77) - this is achieved, but it's worth confirming that the downstream tick() function at line 142 doesn't need to know how many bumps were consolidated.

Observation: The tick() function doesn't take any parameters from the bump stream, so this is fine. The bumps are just wake signals.

4. Minor: Comment could be more precise (worker.rs:77)

The comment "multiple bumps in a row should be processed as 1 bump" is slightly imprecise. It's more accurate to say "multiple bumps are batched together and processed as a single tick."

🎯 Suggested Improvements

1. Clarify metric scope: Document whether this metric should capture all ticks or only bump-triggered ticks
2. Configuration: Consider making the 1024 chunk size configurable
3. Comment precision: Update line 77 comment to be more precise about batching behavior

🔒 Security & Performance

• Security: No security concerns identified
• Performance: The ready_chunks(1024) is a performance optimization that prevents the worker from being overwhelmed by rapid bump messages. This is good. The metric observation overhead is minimal.

🧪 Testing

No tests were added with this change. Consider:

• Unit test to verify the metric is recorded correctly
• Integration test to verify bump batching behavior works as expected

✨ Overall Assessment

This is a good observability improvement with one potential logic issue around metric recording scope. The implementation is clean and follows Rust best practices. Once the metric scope question is clarified, this should be good to merge.

Verdict: ✅ Approve with minor suggestions - The core change is sound, but consider addressing the metric scope issue (whether to track all ticks or just bump-triggered ticks).

Merge activity

• Jan 14, 11:40 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
• Jan 14, 11:41 PM UTC: CI is running for this pull request on a draft pull request (#3908) due to your merge queue CI optimization settings.
• Jan 14, 11:42 PM UTC: Merged by the Graphite merge queue via draft PR: #3908.